feat(planning): badge the interview page title while the tab is hidden - #5522

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Claude finished @kyle-sexton's task in 1m 5s —— View job

Security review complete

• Gather context (PR diff, changed files)
• Run /review:security-review skip gate
• Review changed files for security-relevant surface
• Post findings

No security issues found.

This PR adds a hidden-tab title badge to the planning surface interview page (plugins/planning/surface/index.html) plus matching test/doc/changelog/version updates. Reviewed against the criteria in /review:security-review:

• The new count (away) is derived entirely from local client state (unseenActs().length, noteThread() timestamps vs. S.noteSeen) — no new server endpoint, no new user input, no new data flow across a trust boundary.
• document.title = (away > 0 ? "(" + away + ") " : "") + (m.title || "Interview") assigns a plain string to document.title, which the browser treats as text, not markup — no injection vector even though m.title originates from stored interview data (and that pre-existing assignment path is unchanged by this diff, just relocated).
• No changes to auth, secrets, file paths, or command execution.
• Non-code changes (plugin.json version bump, CHANGELOG.md, README.md) are documentation-only.
• No GitHub Actions files touched.
• No instruction-surface files (CLAUDE.md/AGENTS.md/rules/skill or agent bodies) touched, so the instruction-exception-register lens does not apply here.

Note: this checkout is shallow (git diff origin/main...HEAD fails with "no merge base"), so the review was done by reading the actual feature commits (65d666c, e5078ff, 1226de4) directly rather than a single merge-base diff; the file set matches the PR's changed-files list.

Claude finished @kyle-sexton's task in 4m 0s —— View job

Code review complete

• Gather context (PR diff, files changed)
• Understand request: invoke /review:code-review for this PR
• Run the code-review skill/procedure
• Post inline comments for anchored findings
• Summarize cross-file findings and results

Summary

Reviewed the diff (gh pr diff 5522, scoped to changed files) against this repo's
/review:code-review criteria: correctness, error handling, test coverage, maintainability.
No security-lane items found (and none would apply here regardless).

Two findings posted as inline comments on plugins/planning/surface/index.html:

1. Line 1638 — The badge math (away = un + unread - S.hiddenBase) assumes unseen counts only grow while hidden, but if the Notes or Activity fly-out panel is left open/pinned when the tab is hidden, apply()'s periodic poll/SSE updates still call renderFly() → renderActivity()/renderNotes() regardless of visibility, which silently mark new entries "seen" and undercount or suppress the badge. This path isn't covered by the new ui_journey.js phases 8/9, which never open either panel before hiding the tab.
2. Line 459 — S.hiddenBase is only ever seeded by the visibilitychange listener, so a page opened while already hidden (background tab) never gets a badge until it cycles visible→hidden once.

Both are genuine gaps in the feature as described ("while the tab is hidden the title now shows a count"), not style nits — I did not attempt fixes per this lane's scope (code review, not implementation).

Note: I did not independently run surface.test.sh or the other verification commands the PR body lists; those are author-claimed and unverified by me. The rest of the diff (CHANGELOG.md, plugin.json, README.md, surface.test.sh loop-count edits) looked correct and consistent with the code change.

Claude finished @kyle-sexton's task in 2m 30s —— View job

Security review complete

• Gather context (PR diff, changed files)
• Run /review:security-review skip gate and criteria
• Review changed files for security-relevant surface
• Post findings

No security issues found.

Reviewed the current net diff against origin/main at HEAD 33e1f38 (the mailbox-style gh pr diff --patch output omits merge commit a356b568f's changes since merges don't generate their own format-patch hunks, so I additionally diffed origin/main HEAD directly per file to confirm I had the true current state, including that commit's fixes).

Changed surface, all in plugins/planning/surface/ plus doc/version bumps:

• index.html — the new title-badge logic (arrivalKeys(), S.hiddenBase, the away count) is derived entirely from local, in-memory client state (activity/note entries already loaded into S.doc/S.resp). No new server endpoint, no new user-controlled input, no new data flow across a trust boundary.
• The document.title assignment (L779-L780) builds a plain string ("(" + away + ") " + (m.title || "Interview")) and assigns it to document.title, a property the browser always treats as text — not markup — so this isn't an injection vector even though m.title originates from stored interview data; that assignment path itself is unchanged from before this PR, just relocated and prefixed.
• The visibilitychange listener and the S.hiddenBase seed-on-load path (L1644-L1648) only read local document/state and set local state — no auth, secrets, file paths, or command execution touched.
• surface.test.sh / tests/ui_journey.js changes are test-only (new journey phases 8-11 covering the visible/hidden and open-flyout cases).
• plugin.json (version bump), CHANGELOG.md, README.md are documentation-only.
• No GitHub Actions files touched.
• No instruction-surface files (CLAUDE.md/AGENTS.md/rules/skill or agent bodies) touched, so the instruction-exception-register lens does not apply.

This confirms the earlier security pass on commit 1226de4 (no issues found) still holds for the two commits added since (a356b568f's flyout/background-tab tracking fix, and 33e1f38's duplicate-version-key cleanup): both are logic/data fixes over already-local, already-reviewed state, with no new trust-boundary or injection surface.

Claude finished @kyle-sexton's task in 4m 13s —— View job

Code review complete

• Gather context (PR diff, changed files, prior review threads)
• Invoke /review:code-review procedure for this PR
• Review changed files against that procedure's criteria
• Post inline comments for anchored findings
• Summarize cross-file findings and results

Summary

Reviewed the current head (33e1f38) against origin/main (this checkout is shallow, so I read the accumulated diff across the feature commits plus the live file content rather than a single merge-base diff, and cross-checked the isolated per-commit patch to confirm which hunks are actually this PR's vs. concurrent main churn merged into the branch).

The two edge cases flagged in the earlier review round are fixed, verified against the current code, not just the author's replies:

• Flyout-open undercounting (Codex + Claude): renderHeader() now derives away from arrivalKeys() — a content-keyed Set diffed against a hiddenBase snapshot (index.html:779) — rather than the old seen-count subtraction, so an open Activity/Notes flyout marking entries seen no longer erases the badge. ui_journey.js phases 10/11 exercise exactly this (open panel, hide, new activity arrives, badge still shows).
• Initially-hidden tab (Claude): the first apply() now seeds S.hiddenBase from arrivalKeys() when document.hidden is already true (index.html:1601).

One finding posted inline on plugins/planning/surface/index.html:1601: that second fix (background-tab load) has no test anywhere in the suite. The author's reply pointed to journey phases 10/11 for this finding too, but those phases cover the flyout-open case, not a page whose document.hidden is already true at first load — a distinct code path with zero coverage. Not a correctness bug in the current diff, but a concrete, addressable gap on behavior the PR's own fix commit calls out by name.

Rest of the diff (plugin.json version bump, CHANGELOG.md, README.md sentence, surface.test.sh loop-count/skip-count edits) is internally consistent — no duplicate version keys, changelog ordering intact, the 1..11 phase loop and SKIP: 294 ... 110 counts in surface.test.sh match the phases actually defined in ui_journey.js.

Per this lane's scope, I did not independently re-run surface.test.sh / check-changelog-parity.sh / validate-plugins.sh; the PASS=294/FAIL=0/SKIP=1 in the PR body is author-claimed and unverified by me. No security-relevant surface in this diff (client-only state, no new endpoints, no document.title injection vector — plain string assignment) and a security lane already ran and posted "no issues found" on this PR, so I did not duplicate that pass.
· branch feat/5473-hidden-tab-title-badge

Claude finished @kyle-sexton's task in 3m 40s —— View job

Code review complete

• Gather context (PR diff, changed files, prior review threads)
• Run /review:code-review skip gate
• Review changed files against this lane's criteria
• Post inline comments for anchored findings
• Summarize cross-file findings and results

Summary

Reviewed the current head (68eb59e) against this lane's criteria (correctness, error handling, test coverage, maintainability; security is out of scope here since a dedicated security lane already ran on this PR).

No new findings. The merge commit 68eb59e (origin/main → this branch) touches only unrelated files elsewhere in the repo — it makes no changes to plugins/planning/surface/** or the other four changed files, so the feature diff at this head is identical to what the prior review rounds already covered at 33e1f38.

Checked that the two substantive gaps raised earlier in this thread are actually fixed in the current code, not just claimed:

• Flyout-open undercounting (index.html:779): away is now derived from arrivalKeys() (index.html:602), a content-keyed snapshot diffed against S.hiddenBase, independent of whatever markSeen()/noteSeen an open Activity or Notes flyout advances while the tab is hidden. Confirmed by reading the code, not just the commit message.
• Initially-hidden tab has no baseline (index.html:1602): the first apply() now seeds S.hiddenBase = arrivalKeys() when document.hidden is already true, and this path now has dedicated coverage — journey phases 12/13 (tests/ui_journey.js:344-355) reload the page with document.hidden forced true before first load, then assert the badge appears once activity lands. This closes the test-coverage gap flagged in the prior round on the same line.

Rest of the diff (plugin.json version bump, CHANGELOG.md entry, README.md sentence, surface.test.sh phase/skip-count edits) is internally consistent with the code and with the post-merge changelog ordering (this entry now sits at 0.48.2, correctly reordered above the concurrently-merged 0.48.1 entry from main).

Not independently re-run: surface.test.sh, check-changelog-parity.sh, validate-plugins.sh. The PR body's PASS/FAIL/SKIP counts are author-claimed and unverified by me; note they predate the phase 12/13 additions and no longer match the skip count in the current surface.test.sh (298, not 294) — likely just a stale body, not a code issue, so not raised as a finding.
· branch feat/5473-hidden-tab-title-badge

Claude finished @kyle-sexton's task in 2m 4s —— View job

Security review complete

• Gather context (PR diff, changed files)
• Invoke /review:security-review per custom instructions
• Review changed files for security-relevant surface
• Post findings

No security issues found.

Reviewed the current head (68eb59e407dcf66e40e540777120c1d566e78231) against the skill's criteria (logic/trust-boundary flaws, injection, auth, secrets; Actions hardening deferred to zizmor). The checkout's shallow history means gh pr diff --patch (mailbox/format-patch) silently drops the changes carried inside the merge commit a356b56 — this branch resolved a merge conflict there by folding in the flyout-independent-counting fix (arrivalKeys()), so I read the live file content plus git show a356b56 -- plugins/planning/surface/index.html to confirm the actual current code rather than trusting the incomplete patch series.

Security-relevant surface, all in plugins/planning/surface/index.html:

• The new title-badge state (arrivalKeys() at L600, S.hiddenBase, the away count at L779) is derived entirely from local, already-loaded client state (S.doc.activity, note thread). No new server endpoint, no new user-controlled input, no new data flow across a trust boundary.
• document.title = (away > 0 ? "(" + away + ") " : "") + (m.title || "Interview") (L780) assigns a plain string to document.title, which the browser always treats as text, never markup — not an injection vector even though m.title originates from stored interview data; that assignment path is unchanged from before this PR, just relocated and prefixed.
• The visibilitychange listener and the load-time seed for an already-hidden tab (L1601, L1644-L1648) only read and write local document/state.
• surface.test.sh / tests/ui_journey.js changes (phases 8-13) are test-only, run against a local fixture server; the Object.defineProperty(document, ...) overrides only fake visibility state inside the Playwright test page, no production-reachable path.
• plugin.json (version bump), CHANGELOG.md, README.md are documentation-only.
• No GitHub Actions files touched. No instruction-surface files (CLAUDE.md/AGENTS.md/rules/skill or agent bodies) touched, so the instruction-exception-register lens does not apply.

This confirms the prior security passes on this feature (commits 1226de4, 33e1f38) still hold, and additionally verifies the merge-commit fix (a356b56) and the newest test-only commit (eb9d803) that a plain diff tool missed.
· branch feat/5473-hidden-tab-title-badge

Claude finished @kyle-sexton's task in 1m 2s —— View job

Security review complete

• Gather context (PR diff, changed files)
• Invoke /review:security-review and follow its instructions exactly
• Review changed files for security-relevant surface
• Post findings

Skip gate: this head already has a successful security review that still applies. No new findings.

Current head is e68576d740099497cd283a98a33e0e9b3ac92803, a merge of origin/main into this branch. Comparing against the last-reviewed head (68eb59e, reviewed 2026-09-30T14:24:15Z, "no security issues found"):

• gh pr diff 5522 confirms the PR's changed-file set is unchanged: plugin.json, CHANGELOG.md, surface/README.md, surface/index.html, surface.test.sh, tests/ui_journey.js.
• git diff 68eb59e..e68576d for plugins/planning/surface/index.html shows only unrelated changes from the merged-in origin/main commit (5eb1b99, "let interview revise update a question's commitments") — confirmed() gained a contentRev/commitsSinceSeq check, and confirmCommit() now sends contentRev — neither touches the title-badge feature this PR adds.
• The badge logic itself — arrivalKeys(), the S.hiddenBase seed/reset in the visibilitychange listener and first-apply() branch (L1602, L1645-L1649), and the document.title assignment (L779-L780) — is byte-identical to what was reviewed at 68eb59e: local, already-loaded client state only, no new server endpoint, no new user-controlled input, no new trust boundary, and a plain string assigned to document.title (never markup, so no injection vector).
• plugin.json (version bump), CHANGELOG.md, README.md are documentation-only. surface.test.sh / tests/ui_journey.js changes are test-only, unchanged from the prior review.

Per the skip gate ("this head already has a successful security review that still applies"), no further review was performed and nothing new is posted.
· branch feat/5473-hidden-tab-title-badge

Claude finished @kyle-sexton's task in 1m 18s —— View job

Code review complete

• Gather context (PR diff, changed files, prior review threads)
• Invoke /review:code-review and follow its instructions
• Review changed files against that lane's criteria
• Post inline comments for anchored findings
• Summarize cross-file findings and results

Summary

Skip gate hit: this head already has a successful review from this lane that still applies.

Current head e68576d740099497cd283a98a33e0e9b3ac92803 is a merge of origin/main into this branch, landing on top of 68eb59e, which this lane already reviewed in full and found no findings. I confirmed the merge changed nothing this lane cares about, rather than trusting that claim:

• git diff origin/main...HEAD for the PR's own changed files (plugin.json, CHANGELOG.md, surface/README.md, surface/index.html, surface/surface.test.sh, surface/tests/ui_journey.js) is 74 insertions(+), 10 deletions(-) — byte-identical in size to the PR body's stated diff, confirming the merge introduced no new lines to this PR's own diff.
• git diff 68eb59e e68576d -- plugins/planning/surface/index.html shows the merge did pull in unrelated changes from origin/main (the commits/commitsSinceSeq confirm-staleness logic from feat(planning): let interview revise update a question's commitments #5497 — confirmed(), confirmCommit(), exporters.py), but a targeted grep of that diff for hiddenBase|arrivalKeys|away|document.title|visibilitychange (this PR's own feature surface) returns nothing: those lines are byte-for-byte unchanged since 68eb59e.
• Same check on surface.test.sh and README.md: the differences between 68eb59e and this head are exclusively the feat(planning): let interview revise update a question's commitments #5497 commitments-feature hunks (a new --commit test line, a contentRev/stale-confirm sentence), not anything touching the title-badge feature or its tests.

So the badge implementation (arrivalKeys() at index.html:602, S.hiddenBase seeding at index.html:1602, away/document.title at index.html:779-780, and the visibilitychange listener at index.html:1645-1649) is exactly the code the two most recent review rounds already covered end-to-end — including verifying that both earlier findings (flyout-open undercounting, initially-hidden-tab baseline) were fixed in code, not just claimed, and that the fixes have dedicated ui_journey.js coverage (phases 10-13).

Per this lane's skip gate, no new review pass or inline comments follow. No security-relevant surface either way (client-only state, no injection vector in the document.title assignment — unchanged from prior passes).
· branch feat/5473-hidden-tab-title-badge

Claude finished @kyle-sexton's task in 47s —— View job

Code review complete

• Gather context (PR diff, changed files, prior review threads)
• Run /review:code-review skip gate
• Review changed files against this lane's criteria
• Post inline comments for anchored findings
• Summarize cross-file findings and results

Summary

Skip gate hit: this head already has a successful review from this lane that still applies.

Current head 8eafe22dbd4435d526fa3ffc8a68581302df4554 is a merge of origin/main into this branch, landing on top of e68576d, which this lane already reviewed in full (2026-09-30T14:39:50Z) and found no findings. Verified this directly rather than trusting the prior claim:

• git diff e68576d..8eafe22 for the PR's own files (plugins/planning/surface/README.md, index.html, surface.test.sh, tests/ui_journey.js) is empty — byte-identical since the last review.
• The only diff is in plugins/planning/.claude-plugin/plugin.json (0.49.1 → 0.49.2) and plugins/planning/CHANGELOG.md, both mechanical: the merge pulled in an unrelated concurrent version bump (0.49.1, the export-brief acceptance-criteria fix, #5461) from main, so this PR's own entry was renumbered to 0.49.2 and re-slotted above it. Checked the resulting CHANGELOG.md for correctness: dates are consistent, entries are in descending-version order, no duplicate version keys — plugins/planning/CHANGELOG.md.

So the badge implementation itself — arrivalKeys(), the S.hiddenBase seed/reset (including the initially-hidden-tab fix and its phase 12/13 test coverage), and the document.title assignment in index.html — is exactly the code the last two review rounds already covered end-to-end, including confirming both earlier findings (flyout-open undercounting, initially-hidden-tab baseline) were fixed in code with dedicated test coverage.

Per this lane's skip gate, no new review pass or inline comments follow. No security-relevant surface either (client-only state, no injection vector in the document.title assignment) — unchanged from prior passes, and a dedicated security lane already covered this PR.
· branch feat/5473-hidden-tab-title-badge

Claude finished @kyle-sexton's task in 1m 19s —— View job

Security review complete

• Gather context (PR diff, changed files, prior review threads)
• Invoke /review:security-review and follow its instructions exactly
• Review changed files for security-relevant surface
• Post findings

Skip gate: this head already has a successful security review that still applies. No new findings.

Current head is 8eafe22dbd4435d526fa3ffc8a68581302df4554, a merge of origin/main into this branch, landing on top of e68576d, which this lane already reviewed in full (2026-09-30T14:39:50Z, "no security issues found"). I verified that claim against the actual diff rather than trusting it:

• git diff e68576d..8eafe22 --stat for the PR's own files (plugins/planning/surface/index.html, surface.test.sh, tests/ui_journey.js, surface/README.md) shows zero changes — the title-badge feature code is byte-identical to what was reviewed at e68576d.
• The merge pulled in five new commits from origin/main (1ce260e, bb7f67e, 9645ab6, e92880c, 03b44d0). Only e92880c ("export-brief carries the restated acceptance criteria", fix(planning): export-brief carries the restated acceptance criteria #5488) touches anything under plugins/planning/, and it lands in plugins/planning/surface/exporters.py and test_exporters.py — files this PR does not touch and that are unrelated to the hidden-tab badge (a separate export-formatting fix for acceptance-criteria bullets, already merged to main on its own PR).
• plugin.json and CHANGELOG.md changed only to reorder/renumber this PR's changelog entry (0.49.1 → 0.49.2) above the newly-merged export-brief fix entry — a version-bump/reorder, not a content change to this PR's own entry.

So the badge implementation — arrivalKeys(), S.hiddenBase seeding (including the initially-hidden-tab fix at index.html:1602), the away/document.title assignment at index.html:779-780, and the visibilitychange listener — is exactly the code the last three review rounds already covered end-to-end: local, already-loaded client state only, no new server endpoint, no new user-controlled input, no new trust boundary, and a plain string assigned to document.title (never markup, so no injection vector).

Per the skip gate ("this head already has a successful security review that still applies"), no further review pass or new findings follow.
· branch feat/5473-hidden-tab-title-badge

Claude finished @kyle-sexton's task in 32s —— View job

Security review complete

• Gather context (PR diff, changed files, prior review threads)
• Run /review:security-review skip gate
• Review changed files for security-relevant surface
• Post findings

Skip gate: this head already has a successful security review that still applies. No new findings.

Current head is bd7d394417c3dff9177f4f65360437725cb0d7c8, a merge of origin/main into this branch, landing on top of 8eafe22d, which this lane already reviewed in full (2026-09-30T15:05:32Z, "no security issues found"). Verified that against the actual diff rather than trusting it:

• git diff 8eafe22d..bd7d394 -- plugins/planning/surface/ is empty — the title-badge feature code (index.html, README.md, surface.test.sh, tests/ui_journey.js) is byte-identical to what was reviewed at 8eafe22d.
• The merge pulled in ~136 files from origin/main, all unrelated to this PR (repo-hygiene tooling, changelog-skill fixtures, disk-hygiene scripts, source-control hooks, various plugin SKILL.md/doc touch-ups). None touch plugins/planning/surface/**.
• plugins/planning/.claude-plugin/plugin.json changed only 0.49.2 → 0.49.3 (mechanical bump), and plugins/planning/CHANGELOG.md only re-slotted this PR's existing entry above a newly-merged, unrelated 0.49.2 entry (a Boundary-bullet wording change in draft-goal-condition/plan, no code) — no content change to this PR's own entry.

So the badge implementation — arrivalKeys(), S.hiddenBase seeding (including the initially-hidden-tab fix), the away/document.title assignment, and the visibilitychange listener in index.html — is exactly the code the prior five review rounds already covered end-to-end: local, already-loaded client state only, no new server endpoint, no new user-controlled input, no new trust boundary, and a plain string assigned to document.title (never markup, so no injection vector).

Per the skip gate ("this head already has a successful security review that still applies"), no further review pass or new findings follow.
· branch feat/5473-hidden-tab-title-badge

Claude finished @kyle-sexton's task in 32s —— View job

Code review complete

• Gather context (PR diff, changed files, prior review threads)
• Run /review:code-review skip gate
• Review changed files against this lane's criteria
• Post inline comments for anchored findings
• Summarize cross-file findings and results

Summary

Skip gate hit: this head already has a successful review from this lane that still applies.

Current head bd7d394417c3dff9177f4f65360437725cb0d7c8 is a merge of origin/main into this branch, landing on top of 8eafe22, which this lane already reviewed in full (2026-09-30T15:05:32Z) and found no findings. Verified this directly rather than trusting the prior claim:

• git diff 8eafe22..bd7d394 for the PR's own files (plugins/planning/surface/index.html, surface.test.sh, tests/ui_journey.js, surface/README.md) is empty — the title-badge feature code is byte-identical to what was reviewed at 8eafe22.
• The merge pulled in twelve new commits from origin/main (9f9b055 … 023026f), none of which touch anything under plugins/planning/.
• plugin.json and CHANGELOG.md changed only to reorder/renumber this PR's changelog entry (0.49.2 → 0.49.3) above a newly-merged, unrelated entry (0.49.2, the native-Boundary-bullet fix, #5568) — a mechanical version bump, not a content change to this PR's own entry. Checked the resulting CHANGELOG.md for correctness: descending-version order intact, no duplicate version keys, dates consistent.

So the badge implementation itself — arrivalKeys(), the S.hiddenBase seed/reset (including the initially-hidden-tab fix and its dedicated phase 12/13 test coverage), and the document.title assignment in index.html — is exactly the code the last several review rounds already covered end-to-end, including confirming both earlier findings (flyout-open undercounting, initially-hidden-tab baseline) were fixed in code with dedicated test coverage.

Per this lane's skip gate, no new review pass or inline comments follow. No security-relevant surface either (client-only state, no injection vector in the document.title assignment) — unchanged from prior passes, and a dedicated security lane already covered this PR.
· branch feat/5473-hidden-tab-title-badge